Skip to content

Conversation

@dingfeli
Copy link
Contributor

Issue #, if available:
#1856
The TLDR is that because tool list calls are now done passively, early errors with mcp server load is terminating the mcp server loading status task prematurely.

Description of changes:

  • Precompuates total number of servers (which are used to determine when to terminate the mcp loading status task) prior to spawning the task
  • Changes display task to use async task as opposed to spawning a dedicated blocking thread. This was previously needed because we were holding onto a stdout lock. But we are no longer doing that, thus rendering the dedicated blocking thread unnecessary.
  • Adds /mcp slash command to show server load records.

Testing some workflows:

  • Load in interactive mode with correct config with timeout set to max
    • Everything should be loaded before conversation starts
image
  • Load in interactive mode with incorrect config with timeout set to max
    • Status should still show
    • Error associated with incorrect config should be shown
    • Fast returning error (i.e. incorrect bin name) should not terminate status display
image
  • Load in interactive mode with correct config with timeout set to default (5s)
    • Servers that do not load within the timeout should be displayed under "servers still loading" section
image
  • Load in interactive mode with incorrect config with timeout set to default (5s)
    • Servers that do not load within the timeout should be displayed under "servers still loading" section
    • Errors should be displayed in the status
image
  • Load in no-interactive mode with correct config with no-interactive timeout set to max
    • Status should not show
    • Everything should be loaded before conversation starts
image
  • Load in no-interactive mode with incorrect config with no-interactive timeout set to max
    • Status should only show errors
image
  • Load in no-interactive mode with correct config with timeout set to a shorter time (5s)
    • Success status should not show
    • Warning should show for that the fact that some servers did not load
    • Actual servers should be logged
image
  • Load in no-interactive mode with incorrect config with timeout set to a shorter time (5s)
    • Success status should not show
    • Errors should show to notify users that some servers did not load properly
    • Actual servers should be logged
image

Added tip:
image

/mcp with partial loaded servers:
image

/mcp with all servers loaded:
image

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.75%. Comparing base (15a489c) to head (0481db8).
Report is 16 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1919   +/-   ##
=======================================
  Coverage   16.75%   16.75%           
=======================================
  Files         213      213           
  Lines       20704    20704           
  Branches      871      871           
=======================================
  Hits         3468     3468           
  Misses      17236    17236           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dingfeli dingfeli merged commit 9dd4f27 into main May 20, 2025
21 checks passed
@dingfeli dingfeli deleted the fix-mcp-loading-status branch May 20, 2025 23:07
brandonskiser pushed a commit that referenced this pull request May 21, 2025
* precomputes total number of servers in loading prior to spawning display task

* changes display task to use async task instead of spawn blocking

* waits on notify instead of display task for initial loading

* includes time taken in warning and error

* adds slash command to show mcp server load messages

* includes mcps that fail to start in /mcp

* surfaces command error for mcp servers in non-interactive mode

* only surfaces timeout warning in non-interactive mode with one or more server in the client list

* adds copy change for /mcp

* uses a hash set of server names to keep track of the number of initialized servers

* only shows mcp non-interactive msg in non-interactive mode
This was referenced May 21, 2025
hayemaxi pushed a commit to hayemaxi/amazon-q-developer-cli that referenced this pull request May 22, 2025
* precomputes total number of servers in loading prior to spawning display task

* changes display task to use async task instead of spawn blocking

* waits on notify instead of display task for initial loading

* includes time taken in warning and error

* adds slash command to show mcp server load messages

* includes mcps that fail to start in /mcp

* surfaces command error for mcp servers in non-interactive mode

* only surfaces timeout warning in non-interactive mode with one or more server in the client list

* adds copy change for /mcp

* uses a hash set of server names to keep track of the number of initialized servers

* only shows mcp non-interactive msg in non-interactive mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants